Skip to content

Skins: fix time display to allow AM/PM - #13430

Merged
ywwg merged 1 commit into
mixxxdj:2.4from
ronso0:skins-fix-time-display
Aug 1, 2024
Merged

Skins: fix time display to allow AM/PM#13430
ywwg merged 1 commit into
mixxxdj:2.4from
ronso0:skins-fix-time-display

Conversation

@ronso0

@ronso0 ronso0 commented Jul 1, 2024

Copy link
Copy Markdown
Member

I removed the fixed hh:mm format which allows AM/PM and adjusted the size policies and the parent WidgetGroups so AM/PM is not cut off.

Please double check each skin.

Fixes #13421

@github-actions github-actions Bot added the skins label Jul 1, 2024
@ronso0 ronso0 linked an issue Jul 1, 2024 that may be closed by this pull request
@daschuer

daschuer commented Jul 2, 2024

Copy link
Copy Markdown
Member

Looks good. However I like the 24:00 format more in Shade, I can however not imagine if it is readable if you are not used to it.

@ronso0

ronso0 commented Jul 2, 2024

Copy link
Copy Markdown
Member Author

However I like the 24:00 format more in Shade

This is not enforcing AM/PM. By default, WTime uses the system's time locale. So I assume @RafaelFrance has that set in his system. @RafaelFrance can you confirm?
I use en_GB for lang, all other settings (time, measures) are de_DE, and I had to enforce AM/PM in order to see it in Mixxx.
I have reset my system clock to hh:mm and that's how it looks like in Mixxx now.

@RafaelFrance

Copy link
Copy Markdown
Contributor

Yes, when using Mixxx, I use OS system format (24:00 hour format, French language). No problem.

But when testing Mixxx, I change to English language in Mixxx preferences, in order to report issues with correct wording. I think that here I set English (United States) format.

@ronso0

ronso0 commented Jul 2, 2024

Copy link
Copy Markdown
Member Author

Thanks for verifying!
Do you have time to test the CI build from https://github.com/mixxxdj/mixxx/actions/runs/9751678277?pr=13430#artifacts?

@daschuer

daschuer commented Jul 3, 2024

Copy link
Copy Markdown
Member

Interesting could be the opinion of English speaking users. Do they prefer the military time for a compact display or the AM/PM format which is more common.

@daschuer

daschuer commented Jul 3, 2024

Copy link
Copy Markdown
Member

@ywwg ?

@RafaelFrance

RafaelFrance commented Jul 3, 2024

Copy link
Copy Markdown
Contributor

Tested. Git Version : 2.4.1-36-ga9b6791259 (HEAD).

AM/PM is absent. Only "10:02" in place of "10:02 PM".

@ronso0

ronso0 commented Jul 3, 2024

Copy link
Copy Markdown
Member Author

cross-posting from #13421:

I can verifiy AM/PM is used when I pick the en_US locale in Mixxx. You not seeing AM/PM must be a locale issue. What locale do you use for Mixxx when you expect AM/PM?

@RafaelFrance

Copy link
Copy Markdown
Contributor

My laptop system is : French.

When testing I used English (United States) local in Preference/Interface of Mixxx.
It is the same setting that when I found the issue on #13421

Note : when I use Mixxx I do not have any problem because Mixxx is set on "System" and hours are in 24h.

@ronso0

ronso0 commented Jul 4, 2024

Copy link
Copy Markdown
Member Author

Well, that means something else (not Mixxx) is not as before, because in Tango I only changed the size not the time config (other skins now allow AP/PM, too)
https://github.com/mixxxdj/mixxx/pull/13430/files#diff-014090a33661d1e7219e18239a7a9482c2f6d4a0436ad5c43cd7ebedfbf7eb7f

@ronso0 ronso0 changed the title Skins: fix time display Skins: fix time display to allow AM/PM Jul 5, 2024
@ronso0
ronso0 marked this pull request as ready for review July 31, 2024 00:00
@ywwg

ywwg commented Jul 31, 2024

Copy link
Copy Markdown
Member

thanks for this! I should have looked first

@ronso0

ronso0 commented Jul 31, 2024

Copy link
Copy Markdown
Member Author

So, let's merge?

@ywwg

ywwg commented Jul 31, 2024

Copy link
Copy Markdown
Member

is it worth fixing this for 2.4? it's not a huge issue and backporting is a pain, so how about we target 2.5? Not a strong opinion

@ronso0

ronso0 commented Jul 31, 2024

Copy link
Copy Markdown
Member Author

I don't understand, this is already targeting 2.4 and, as far as I'm aware, the toolbars and mixer in Shade didn't change recently, so merging 2.4 into 2.5 will be easy.

@ywwg
ywwg merged commit ec076cd into mixxxdj:2.4 Aug 1, 2024
@ronso0
ronso0 deleted the skins-fix-time-display branch August 1, 2024 02:14
@ronso0 ronso0 added this to the 2.4.2 milestone Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Clock display incomplet

4 participants